Update cell layout and behavior for obfuscation methods #7020

rablador · 2024-10-18T14:28:11Z

For each cell that needs it (udp2tcp, shadowsocks):

the cell should display the current port setting
the cell should have an action button that will eventually navigate to a new view
there should be a divider
It should use the correct background color when selected/unselected

This change is

linear · 2024-10-18T14:28:14Z

IOS-878 Change each cell to show the new behavior

acb-mv

Reviewed 12 of 12 files at r1, all commit messages.
Reviewable status: complete! all files reviewed, all discussions resolved

pinkisemils

Tested it out in mock release, and it is fine - no changes. Debug build looks to spec. Have not checked the code itself.

Reviewable status: complete! all files reviewed, all discussions resolved

rablador

Updated color of separator.

Reviewable status: 11 of 12 files reviewed, all discussions resolved (waiting on @acb-mv)

mojganii

Reviewable status: 11 of 12 files reviewed, 4 unresolved discussions (waiting on @acb-mv and @rablador)

ios/MullvadVPN/UI appearance/UIMetrics.swift line 96 at r2 (raw file):

        static let apiAccessSwitchCellTrailingMargin: CGFloat = apiAccessInsetLayoutMargins.trailing - 4
        static let apiAccessPickerListContentInsetTop: CGFloat = 16
        static let buttonSeparatorHeight: CGFloat = 22

nit : VerticalDeviderHeightcan be a better candidate. Divider color is not clear for unselected options but it's discussed just I put that here to give feedback.

___ *[`ios/MullvadVPN/View controllers/Settings/SelectableSettingsDetailsCell.swift` line 20 at r2](https://reviewable.io/reviews/mullvad/mullvadvpn-app/7020#-OAHY4PO8t3QDd27dWTO:-OAHY4PO8t3QDd27dWTP:b-hsym00) ([raw file](https://github.com/mullvad/mullvadvpn-app/blob/50d9a7b84a8b2e6f8b91e42a7491acf327eeaa07/ios/MullvadVPN/View%20controllers/Settings/SelectableSettingsDetailsCell.swift#L20)):* > ```Swift > > let actionButton = IncreasedHitButton(type: .system) > actionButton.setImage(UIImage(systemName: "ellipsis"), for: .normal) > ```

let's use UIButton.Configuration for button styling.

ios/MullvadVPN/View controllers/VPNSettings/VPNSettingsCellFactory.swift line 174 at r2 (raw file):

            #if DEBUG
            cell.detailTitleLabel.text = String(format: NSLocalizedString(
                "WIRE_GUARD_OBFUSCATION_SHADOWSOCKS_PORT",

Wireguard is a name then texts should be started with Wireguard_

ios/MullvadVPN/View controllers/VPNSettings/VPNSettingsDetailsButtonItem.swift line 10 at r2 (raw file):

enum VPNSettingsDetailsButtonItem {
    case udpTcp

I recommend avoid abbreviation as much as possible , udpOverTcp and wireguardOverShadowsocks

mojganii

there are some codes in UITest that is looking wireGuardObfuscationOn which is no longer available, they need to attention to be resolved

Reviewable status: 11 of 12 files reviewed, 4 unresolved discussions (waiting on @acb-mv and @rablador)

rablador

Reviewable status: 11 of 12 files reviewed, 4 unresolved discussions (waiting on @acb-mv and @mojganii)

ios/MullvadVPN/UI appearance/UIMetrics.swift line 96 at r2 (raw file):

Previously, mojganii wrote…

nit : VerticalDeviderHeightcan be a better candidate. Divider color is not clear for unselected options but it's discussed just I put that here to give feedback.

Feedback noted, but I think you should add it as a comment on the designs instead so that it reaches the right person.

ios/MullvadVPN/View controllers/Settings/SelectableSettingsDetailsCell.swift line 20 at r2 (raw file):

Previously, mojganii wrote…

let's use UIButton.Configuration for button styling.

Done.

ios/MullvadVPN/View controllers/VPNSettings/VPNSettingsCellFactory.swift line 174 at r2 (raw file):

Previously, mojganii wrote…

Wireguard is a name then texts should be started with Wireguard_

Done.

ios/MullvadVPN/View controllers/VPNSettings/VPNSettingsDetailsButtonItem.swift line 10 at r2 (raw file):

Previously, mojganii wrote…

I recommend avoid abbreviation as much as possible , udpOverTcp and wireguardOverShadowsocks

Done.

buggmagnet

Not sure what happened here, but the offset that used to be to the right of the version string seems to be missing now.
I have checked on main, and the offset looks good there.

Reviewed 4 of 12 files at r1, 7 of 8 files at r3, all commit messages.
Reviewable status: 13 of 14 files reviewed, 8 unresolved discussions (waiting on @mojganii and @rablador)

a discussion (no related file):
I can trigger two obfuscation settings being enabled at the same time. I'm guessing it's because we don't save anything just yet when we select some obfuscation ?
RPReplay_Final1730130143.mov

ios/MullvadVPN/View controllers/Settings/SelectableSettingsCell.swift line 47 at r3 (raw file):

                tickImageView.pinEdgesToSuperview(PinnableEdges([
                    .leading(0),
                    .trailing(UIMetrics.SettingsCell.selectableSettingsCellLeftViewSpacing),

nit
can't we just set the ,trailing one leaving the others by default to 0 ?

ios/MullvadVPN/View controllers/Settings/SelectableSettingsDetailsCell.swift line 14 at r3 (raw file):

    let viewContainer = UIView()

    var action: (() -> Void)?

I'm surprised the compiler doesn't warn here.
action is actually a function defined in CALayerDelegate which all UIView subclasses adopt (so does UITableViewCell)

I'm assuming that it doesn't complain because this is technically a valid function overload, but I really think we should rename this to something more explicit like ellipsisAction to avoid future confusion.

We should do the same in SettingsAddDNSEntryCell

ios/MullvadVPN/View controllers/Settings/SelectableSettingsDetailsCell.swift line 29 at r3 (raw file):

            self,
            action: #selector(didPressActionButton),
            for: .valueChanged

This won't work unless you use .touchUpInside

rablador

Fixed.

Reviewable status: 13 of 14 files reviewed, 8 unresolved discussions (waiting on @buggmagnet and @mojganii)

a discussion (no related file):

Previously, buggmagnet wrote…

I can trigger two obfuscation settings being enabled at the same time. I'm guessing it's because we don't save anything just yet when we select some obfuscation ?
RPReplay_Final1730130143.mov

Both udpTcp and shadowsocks point to .on, so that's why.

ios/MullvadVPN/View controllers/Settings/SelectableSettingsCell.swift line 47 at r3 (raw file):

Previously, buggmagnet wrote…

nit
can't we just set the ,trailing one leaving the others by default to 0 ?

It can (sort of) be done, but it'll look like there's something special going on, given that the assignments have been split up. I've provided a more compact solution in the meantime.

Code snippet:

tickImageView.pinEdgesToSuperview(.all().excluding(.trailing))
tickImageView.pinEdgesToSuperview(PinnableEdges([.trailing(UIMetrics.SettingsCell.selectableSettingsCellLeftViewSpacing)]))

ios/MullvadVPN/View controllers/Settings/SelectableSettingsDetailsCell.swift line 14 at r3 (raw file):

Previously, buggmagnet wrote…

I'm surprised the compiler doesn't warn here.
action is actually a function defined in CALayerDelegate which all UIView subclasses adopt (so does UITableViewCell)

I'm assuming that it doesn't complain because this is technically a valid function overload, but I really think we should rename this to something more explicit like ellipsisAction to avoid future confusion.

We should do the same in SettingsAddDNSEntryCell

Done.

ios/MullvadVPN/View controllers/Settings/SelectableSettingsDetailsCell.swift line 29 at r3 (raw file):

Previously, buggmagnet wrote…

This won't work unless you use .touchUpInside

Late copy/paste... 😕

buggmagnet

Reviewed 2 of 12 files at r1, 1 of 8 files at r3, 6 of 6 files at r4, all commit messages.
Reviewable status: all files reviewed, 6 unresolved discussions (waiting on @mojganii and @rablador)

ios/MullvadVPN.xcodeproj/project.pbxproj line 469 at r4 (raw file):

		7A27E3CB2CAE861D0088BCFF /* SettingsViewModel.swift in Sources */ = {isa = PBXBuildFile; fileRef = 7A27E3CA2CAE86170088BCFF /* SettingsViewModel.swift */; };
		7A27E3CD2CB814EF0088BCFF /* DAITAInfoView.swift in Sources */ = {isa = PBXBuildFile; fileRef = 7A27E3CC2CB814EA0088BCFF /* DAITAInfoView.swift */; };
		7A27E3CF2CBD4A8C0088BCFF /* SelectableSettingsDetailsCell.swift in Sources */ = {isa = PBXBuildFile; fileRef = 7A27E3CE2CBD4A830088BCFF /* SelectableSettingsDetailsCell.swift */; };

Nit
The Settings folder is not ordered by name anymore.

rablador

Reviewable status: all files reviewed, 5 unresolved discussions (waiting on @buggmagnet and @mojganii)

ios/MullvadVPN.xcodeproj/project.pbxproj line 469 at r4 (raw file):

Previously, buggmagnet wrote…

Nit
The Settings folder is not ordered by name anymore.

Fixed!

mojganii

Reviewable status: 15 of 16 files reviewed, 5 unresolved discussions (waiting on @acb-mv, @buggmagnet, and @rablador)

ios/MullvadVPN/UI appearance/UIMetrics.swift line 96 at r2 (raw file):

Previously, rablador (Jon Petersson) wrote…

Feedback noted, but I think you should add it as a comment on the designs instead so that it reaches the right person.

Sure

github-actions · 2024-10-30T08:51:13Z

🚨 End to end tests failed. Please check the failed workflow run.

rablador added the iOS Issues related to iOS label Oct 18, 2024

rablador self-assigned this Oct 18, 2024

rablador force-pushed the change-each-cell-to-show-the-new-behavior-ios-878 branch 4 times, most recently from 2591de0 to 9a401c3 Compare October 24, 2024 10:54

rablador requested review from buggmagnet, mojganii and acb-mv October 24, 2024 10:57

rablador force-pushed the change-each-cell-to-show-the-new-behavior-ios-878 branch from 9a401c3 to 22b078b Compare October 24, 2024 10:59

acb-mv previously approved these changes Oct 24, 2024

View reviewed changes

pinkisemils reviewed Oct 24, 2024

View reviewed changes

rablador dismissed acb-mv’s stale review via 50d9a7b October 24, 2024 13:28

rablador force-pushed the change-each-cell-to-show-the-new-behavior-ios-878 branch from 22b078b to 50d9a7b Compare October 24, 2024 13:28

rablador commented Oct 24, 2024

View reviewed changes

mojganii requested changes Oct 28, 2024

View reviewed changes

mojganii reviewed Oct 28, 2024

View reviewed changes

rablador force-pushed the change-each-cell-to-show-the-new-behavior-ios-878 branch from 50d9a7b to 4e0c1b9 Compare October 28, 2024 15:02

rablador commented Oct 28, 2024

View reviewed changes

buggmagnet requested changes Oct 28, 2024

View reviewed changes

rablador commented Oct 29, 2024

View reviewed changes

rablador force-pushed the change-each-cell-to-show-the-new-behavior-ios-878 branch 2 times, most recently from 5be0f46 to c00c25c Compare October 29, 2024 08:59

buggmagnet previously approved these changes Oct 29, 2024

View reviewed changes

rablador commented Oct 29, 2024

View reviewed changes

rablador dismissed buggmagnet’s stale review via 4799429 October 29, 2024 13:15

rablador force-pushed the change-each-cell-to-show-the-new-behavior-ios-878 branch from c00c25c to 4799429 Compare October 29, 2024 13:15

mojganii approved these changes Oct 29, 2024

View reviewed changes

Update cell layout and behavior for obfuscation methods

80eb27b

buggmagnet force-pushed the change-each-cell-to-show-the-new-behavior-ios-878 branch from 4799429 to 80eb27b Compare October 30, 2024 08:16

buggmagnet merged commit 2da598b into main Oct 30, 2024
10 of 11 checks passed

buggmagnet deleted the change-each-cell-to-show-the-new-behavior-ios-878 branch October 30, 2024 08:47

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Update cell layout and behavior for obfuscation methods #7020

Update cell layout and behavior for obfuscation methods #7020

rablador commented Oct 18, 2024 •

edited by faern

Loading

linear bot commented Oct 18, 2024

acb-mv left a comment

pinkisemils left a comment

rablador left a comment

mojganii left a comment

mojganii left a comment

rablador left a comment

buggmagnet left a comment

rablador left a comment

buggmagnet left a comment

rablador left a comment

mojganii left a comment

github-actions bot commented Oct 30, 2024

Update cell layout and behavior for obfuscation methods #7020

Update cell layout and behavior for obfuscation methods #7020

Conversation

rablador commented Oct 18, 2024 • edited by faern Loading

linear bot commented Oct 18, 2024

acb-mv left a comment

Choose a reason for hiding this comment

pinkisemils left a comment

Choose a reason for hiding this comment

rablador left a comment

Choose a reason for hiding this comment

mojganii left a comment

Choose a reason for hiding this comment

mojganii left a comment

Choose a reason for hiding this comment

rablador left a comment

Choose a reason for hiding this comment

buggmagnet left a comment

Choose a reason for hiding this comment

rablador left a comment

Choose a reason for hiding this comment

buggmagnet left a comment

Choose a reason for hiding this comment

rablador left a comment

Choose a reason for hiding this comment

mojganii left a comment

Choose a reason for hiding this comment

github-actions bot commented Oct 30, 2024

rablador commented Oct 18, 2024 •

edited by faern

Loading